【老万】我看代码审查（三）：实战的细节
本系列往期文章：
（图片由谷歌同事 Manu Cornet 创作，来自 www.bonkersworld.net）
审查代码是一门手艺。光知道原理就行了吗？这么想就 naive 了，要审得好还得要多练，让技巧融化在血液里，落实到行动中。像我平均每天审查5个新的CL，每个从几行到几千行不等，就算平均一个CL 100行，一年大概是12万行吧，在谷歌十多年，100多万行总是有的。听起来不算多，但是考虑到很多CL需要好几轮审查才能通过（见上期文章中百轮大战的例子），有时为了写意见还得做一点调研，了解这个CL的背景和技术细节，工作量其实不小，每天一个多小时。以平均每10行一条意见，每条意见两个回合见分晓计算， 我也超过20万次吐槽，老祥林嫂了。
学手艺跟练乐器一回事，重复固然重要，更重要的是在这个过程中用心，否则就是低水平的重复，轻则无效，重则养成坏习惯。如何才算用心、得法呢？我觉得，审查代码的时候要尽力逼自己提出建设性的意见。不要满足于仅仅检验一个CL的做法是否OK。为此，拿到一个CL的时候，先问自己两个问题：
首先，作者真正想要解决的是什么问题？这里关键词是“真正”。不要想当然地认为每个人都知道自己想要什么（不然人世间也不会有那么多悲剧）。就像我们很多人，年轻的时候爱美女，到年纪大了才发现，其实自己爱的，还是美女......啊，我可能举了个假例子，你们不要被误导啊！正经的例子还是很多的，比如有人费尽心力实现了一个在文件中快速检索数据的B+树，结果发现全部数据不超过1GB，可以在内存一次装下......
编程中为什么会出现这种打错靶子的情况？因为理性思维是很累的，需要收集事实、分析、计算、比较、总结，反复推演确保没有逻辑错误，所以人们做决定经常会根据过往经验，走捷径，用感性和直觉代替理性：老虎来了，快跑！必须说直觉是一项很有用的技能：如果老虎来了你还要先做量化分析，根据老虎的体型、毛色、尺寸、速度、眼神、吼声等信号推断它的饥饿程度、吃人的意向、是不是近视眼，再从距离、路况和自己上次体检的指标推导被它追上扑倒的可能性和后果，那么，你被老虎吃掉也许是为人类进化做出了贡献......但是纯粹基于直觉的决策容易把人带进知识体系的盲区。有的时候，初级工程师学会了一个技能，自以为掌握了天下第一的神功，见人就说你看我会这个，你看我这个B+树写得多好！结果花大力气解决了一个错误的问题，对达到最终目的于事无补或是无关痛痒，甚至南辕北辙。这时，需要有经验有担当有理性有良知的四有审查人站出来，说：同学，其实你要的是中文原版的《金瓶梅》，而不是德文版《金瓶梅》加一本《德汉大词典》；其实你要的是《程序员快速找对象法》，而不是《30天学会面向对象编程》；其实你......
搞清楚要解决的问题之后，再考虑第二个问题：如果是我，会怎么做？可能的话，在细看代码之前就想好这个问题，避免读完别人的代码先入为主，被蒙了一块红布带到坑里还以为自己走在康庄大道上头顶一片艳阳天。心中有数之后，带着一个目的去读代码：找出别人的方案跟你自己的有什么不同。有的时候，你会惊喜地发现，别人的做法比你的强，不愧是蓝翔毕业的！你学到了新技能。有时候，你会奇怪别人的做法为什么如此不合常规，难道是有不可告人的目的？这时不要怕暴露自己的幼稚，大胆请教。作者解释完了，如果你恍然大悟击节赞叹，不妨请同事把他的解释加到注释里：要知道，好代码应该是容易懂的。你看不懂，不要想着是因为自己傻，是你一个人的问题。你要考虑到其他需要维护这块代码的同事 ---- 他们可能也傻，甚至更傻。如何防止一群傻子以后把这精妙的逻辑搞坏呢？第一尽量避免不必要的机灵，把代码写得直白。第二把注释写清楚了，让其他人明白作者的良苦用心。第三加足够的测试，如果有人要砸锅，测试会报警。还有的时候，你会发现你的做法更优，这时你便实现了你做为审查人的价值，可喜可贺。
理论讲了不少了，我们来点实战的例子吧。因为我日常工作使用C++为主，所以例子多是C++的，不过，涉及到的原理在其他语言也是适用的。先看个简单的开胃：
实例一：根据姓名查找复仇者联盟人物。
class Avengers {
public:
const Character& LookUpByName(const std::string& name) {
if (!NameIsValid(name)) {
throw InvalidNameError();
}
return name_to_character[name];
}
...
private:
...
std::map<std::string, Character> name_to_character;
};
我们把名字到 Character 的映射存在一个 std::map 里，查找的时候，先看一看名字是否合法。如果合法的话，就从这个 map 中找到对应的 Character 返回。有错吗？
问题来了：要是一个合法的名字因为某种原因不在这个 map 里，会发生什么？我们来看：这个函数会试图返回 name_to_character[name]；因为 name_to_character 这个 map 里面还没有 name，结果会在 map 里插入一对新的值：它的 key 是 name，value 是一个新建的 Character 对象。然后函数返回的是这个新建的对象。这不是我们想要的！
在 C++ 的 std::map 里面，[ ] 操作不光是查询，还会在查询失败的时候修改这个 map。这里，作者的意图只是要查询，并不想要修改 map，所以这时就应该使用一个符合作者意图的，功能更弱的操作：at()。如果 at() 找不到所要的项目，会扔出一个异常，而不是默默地修改 map。所谓杀鸡勿用牛刀：牛刀固然锋利，也容易割着自己的肉。任何时候，慎用大杀器。
改用 at() 的另一个好处是它允许这个 map 是 const 的，于是我们可以把整个函数标记为 const，这样如果不小心在函数内去修改对象的状态，编译器会报错。这是新版（改动部分用高亮标明）：
class Avengers {
public:
const Character& LookUpByName(const std::string& name) const {
if (!NameIsValid(name)) {
throw InvalidNameError();
}
return name_to_character.at(name);
}
...
private:
...
std::map<std::string, Character> name_to_character;
};
实例二：按照事件的性质分发处理。
// event_dispatcher.h
class EventDispatcher {
public:
void Dispatch(const Event& event);
...
};
// event_dispatcher.cpp
static bool IsInterestingEvent(const Event& event) {
...
}
void EventDispatcher::Dispatch(const Event& event) {
...
if (age > 18) {
if (IsInterestingEvent(event)) { ... }
...
}
}
为了实现这个Dispatch()函数，作者王二定义了一个辅助函数 IsInterestingEvent()。老万在审查的过程中，发现 IsInterestingEvent() 从来没有被任何测试覆盖过，于是提议加几个测试，这样以后 IsInterestingEvent() 的定义不会被不小心改坏。王二很虚心地同意了。这是他的新版：
// event_dispatcher.h
class EventDispatcher {
public:
static bool IsInterestingEvent(const Event& event);
void Dispatch(const Event& event);
...
};
// event_dispatcher.cpp
bool EventDispatcher::IsInterestingEvent(const Event& event) {
...
}
void EventDispatcher::Dispatch(const Event& event) {
...
if (age > 18) {
if (IsInterestingEvent(event)) { ... }
...
}
}
你们看，原来的 IsInterestingEvent() 不是 EventDispatcher 类的成员函数。它只是 Dispatch() 实现细节的一部分，所以被标为 static，只能在 event_dispatcher.cpp 文件中被访问 ---- 这样可以防止泄露实现细节，是模块化编程的标准做法。但是呢，这样一来在测试程序中也无法直接调用 IsInterestingEvent() 了。为了解决这个问题，王二把 IsInterestingEvent() 变成了 EventDispatcher 类的成员函数，而且是公开的。这样就可以直接单元测试 IsInterestingEvent() 了。
这样做真的好吗？
两个问题：第一，这样做扩大了 EventDispatcher 的编程接口（API）。一个类的接口越大，复杂度就越高，用户需要更多的时间去学习，实现者需要更多的精力去维护，而且改进系统的阻力也更大（一旦有人开始依赖于一个接口，就不好修改了）。永远不要轻易扩大系统的API。第二个问题，单元测试一个辅助函数起不到完全的验证作用。我们真正关心的，是 Dispatch() 的行为。IsInterestingEvent() 行为正确又怎样？你能保证 Dispatch() 是在用正确的方式调用它吗？如果有一天 Dispatch() 被改成不再调用 IsInterestingEvent() 了呢？这时你的单元测试还是跑得好好的，但是 Dispatch() 的行为可能已经不正确了。
怎么办？虽然有些时候我们会去直接测试辅助函数，那只是万不得已的权宜之计。如果可能的话，还是通过系统的公开接口去测试为好。具体到这个例子，我们不必把 IsInterestingEvent() 暴露到 event_dispatcher.h 当中，在测试里调用 Dispatch() 就好 ---- 只要我们选好输入参数，就可以让 age 大于 18，从而达到间接调用 IsInterestingEvent() 的目的。
实例三：把文本缓冲区写到文件。
class TextBuffer {
public:
void WriteToFile(const std::string& path) {
... 把 lines 写到 path 所指的文件中 ...
lines.clear();
}
...
private:
std::vector<std::string> lines;
};
一个 TextBuffer （文本缓冲区）对象里存了一堆字符串，然后用户可以调用 WriteToFile() 把这些字符串写到文件中然后清空。问题何在？
上期文章中我们说过，写程序最重要的是抽象。何为抽象？说到底就是给一堆具体的实现细节取一个贴切的，和业务逻辑（business logic）对应的名字，这样在读代码的时候就可以用业务逻辑的词汇来思考，大大提高工作效率和正确率。要达到这个目的，取好名字是关键，否则维护代码的人很容易被一个不准确的名字误导，一不小心就犯下逻辑错误。在这个例子中，WriteToFile() 不是一个好名字，因为它和这个函数干的事情并不吻合：除了把缓冲区里的字符串写进文件之外，它还有一个重要的副作用，就是把缓冲区清空。这样如果连续调用 WriteToFile() 两次的话，第二次什么事情都不会发生，因为缓冲区已经是空的了。这种连续执行多次相当于只执行一次的操作，我们叫它做 idempotent 操作。Idempotent 的中文叫“幂等”，老万第一次见到也是有点迷瞪，后来明白了：就是说不管几次幂（只要>=1）都相等的意思。如果我们不看 WriteToFile() 的实现，光看它的名字，大家觉得执行两遍的后果是什么？相信很多同学会想当然地认为缓冲区的内容会被重复写进文件两遍 ---- 那他们就错了！不过，这怪不得他们，只能怪写函数的人取了个误导的名字。
怎么改这个名字才好呢？这类操作在编程界有个标准的名字：flush（冲马桶）。把东西冲走了，自然就没有东西可冲了。
还有一点，这个函数允许调用者每次传一个不同的文件名。如果没有这样的实际需求，这种灵活性只会增加犯错的可能，没有什么好处。如果一个缓冲区只对应一个输出文件的话，更好的做法是把文件名作为构造函数的参数一次性传进去，然后调用 Flush 操作的时候不再指定，像这样：
class TextBuffer {
public:
explicit TextBuffer(const std::string& path)
: output_path(path) {}
void Flush() {
... 把 lines 写到 output_path 所指的文件中 ...
lines.clear();
}
...
private:
const std::string output_path;
std::vector<std::string> lines;
};
时间不早了，今天就讲到这里吧。代码审查的修炼之路是永无尽头的。咱们下次再见！
往期文章推荐：
长按 - 识别二维码关注“老万故事会”公众号：